Skip to content

gh-121445: C implementation for fnmatch.translate#123181

Closed
picnixz wants to merge 102 commits intopython:mainfrom
picnixz:fnmatch-translate-c-implementation-121445
Closed

gh-121445: C implementation for fnmatch.translate#123181
picnixz wants to merge 102 commits intopython:mainfrom
picnixz:fnmatch-translate-c-implementation-121445

Conversation

@picnixz
Copy link
Member

@picnixz picnixz commented Aug 20, 2024 •
edited
Loading

Smaller PR compared to #121446 (I was too lazy for making a new branch so it's a checkout from that other PR). This only adds the C acceleration part for fnmatch.translate. The improvements brought by this PR are quite good:

Release

+------------------------------------------+-----------------------+-----------------------+
| Benchmark                                | fnmatch-translate-ref | fnmatch-translate-c   |
+==========================================+=======================+=======================+
| abc/[!]b-ac-z9-1]/def/\*?/*/**c/?*[][!]  | 6.09 us               | 1.26 us: 4.84x faster |
+------------------------------------------+-----------------------+-----------------------+
| !abc/[!]b-ac-z9-1]/def/\*?/*/**c/?*[][!] | 6.40 us               | 1.29 us: 4.97x faster |
+------------------------------------------+-----------------------+-----------------------+
| a**?**cd**?**??k***                      | 2.17 us               | 303 ns: 7.16x faster  |
+------------------------------------------+-----------------------+-----------------------+
| a/**/b/**/c                              | 2.00 us               | 286 ns: 7.00x faster  |
+------------------------------------------+-----------------------+-----------------------+
| man/man1/bash.1                          | 3.04 us               | 459 ns: 6.63x faster  |
+------------------------------------------+-----------------------+-----------------------+
| a*b*c*d*e*f*g*h*i*j*k*l*m*n*o**          | 5.14 us               | 685 ns: 7.50x faster  |
+------------------------------------------+-----------------------+-----------------------+
| Geometric mean                           | (ref)                 | 6.26x faster          |
+------------------------------------------+-----------------------+-----------------------+

PGO

+------------------------------------------+-----------------------+-----------------------+
| Benchmark                                | fnmatch-translate-ref | fnmatch-translate-c   |
+==========================================+=======================+=======================+
| abc/[!]b-ac-z9-1]/def/\*?/*/**c/?*[][!]  | 6.15 us               | 1.21 us: 5.07x faster |
+------------------------------------------+-----------------------+-----------------------+
| !abc/[!]b-ac-z9-1]/def/\*?/*/**c/?*[][!] | 6.45 us               | 1.24 us: 5.19x faster |
+------------------------------------------+-----------------------+-----------------------+
| a**?**cd**?**??k***                      | 2.15 us               | 287 ns: 7.49x faster  |
+------------------------------------------+-----------------------+-----------------------+
| a/**/b/**/c                              | 1.94 us               | 269 ns: 7.21x faster  |
+------------------------------------------+-----------------------+-----------------------+
| man/man1/bash.1                          | 3.00 us               | 427 ns: 7.03x faster  |
+------------------------------------------+-----------------------+-----------------------+
| a*b*c*d*e*f*g*h*i*j*k*l*m*n*o**          | 5.07 us               | 636 ns: 7.98x faster  |
+------------------------------------------+-----------------------+-----------------------+
| Geometric mean                           | (ref)                 | 6.56x faster          |
+------------------------------------------+-----------------------+-----------------------+

picnixz added 30 commits July 6, 2024 21:16
@picnixz
add implementation
10a6094
@picnixz
add initial tests
ca03388
@picnixz
add generated objects
adb6ed0
@picnixz
update PC/config.c
e95c255
@picnixz
update Modules/Setup
9b1c20d
@picnixz
update VCX project files
85fa953
@picnixz
fix return value
363ec36
@picnixz
fix typo in pythoncore.vcxproj
42b019f
@picnixz
Update pythoncore.vcxproj.filters
4120a95
@picnixz
Amend un-necessary modifications in test_fnmatch.py
36394bb
@picnixz
update configurations
2c2f9f1
@picnixz
add C implementation
ecf8146
@picnixz
update Python implementation
cb16b6a
@picnixz
update tests
751c069
@picnixz
add generated objects
9258068
@picnixz
re-expose private API
5a7183c
@picnixz
fix implementation?
2a80200
@picnixz
update generated objects
36432e8
@picnixz
FIX BUILD
4a36980
@picnixz
remove interned strings
4881f1c
@picnixz
revert addition
ec5a922
@picnixz
make it fail on Windows for now
b29ccb4
@picnixz
Update configurations?
a91f689
@picnixz
split implementation
9e93b58
@picnixz
regenerated objects
5cfd580
@picnixz
maybe it's like that?
891a368
@picnixz
maybe like this...?
adc18bd
@picnixz
regenerate configure script
658fb81
@picnixz
update tests
cb29bd3
@picnixz
update files
c1fae24
@picnixz picnixz requested review from a team, corona10 and erlend-aasland as code owners August 20, 2024 15:35
@bedevere-app bedevere-app bot mentioned this pull request Aug 20, 2024
Closed
@barneygale
Copy link
Contributor

barneygale commented Aug 20, 2024 •
edited
Loading

Is this worth the added code complexity? Most users interact with translate() via one of the other functions (fnmatch(), fnmatchcase(), filter()), and those functions call translate() via an LRU cache, so the speed of the Python implementation matters less.

@picnixz
Copy link
Member Author

picnixz commented Aug 21, 2024

You're right on this matter. To be honest, I wanted to improve fnmatch in C at that time because it was a simple module and it was also a way to familiarize myself with the C API and its internals (this also helped in writing python/devguide#1350). However, I don't mind not having this feature and understand your concerns (though I wouldn't imagine that we would gain a factor 6.5x in speed...).

Now, as you could see in the other PR, the improvements brought by fnmatch.filter are marginal so we don't really have a user gain, so I won't push for this feature (I'm satisfied with what I learned at least!)

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Aug 23, 2024

Based on the discussion above, I believe we can close this PR?

@erlend-aasland erlend-aasland added the pending The issue will be closed if no feedback is provided label Aug 23, 2024
@picnixz
Copy link
Member Author

picnixz commented Aug 23, 2024

Oh yes. We can close it. I'll keep the branch on my side if I ever want to ressurect it though but yes. Sorry for forgetting about it.

@picnixz picnixz closed this Aug 23, 2024
@picnixz picnixz removed awaiting review pending The issue will be closed if no feedback is provided labels Aug 23, 2024
@picnixz picnixz removed request for a team, corona10 and erlend-aasland August 23, 2024 08:36
@picnixz picnixz deleted the fnmatch-translate-c-implementation-121445 branch November 30, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@picnixz @barneygale @erlend-aasland